-
Notifications
You must be signed in to change notification settings - Fork 24
2581 update messages pallet to support intents #2616
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
2581 update messages pallet to support intents #2616
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made the following enhancements for try-runtime-* targets:
- Only build the runtime, not the entire node; speeds up the build
- Consolidate targets using patterns to avoid duplicated targets/commands
- Fetch state for a minimal set of system pallets so that multi-block migrations can be properly tested
- Allow the use of the
PALLETSenv var to specify additional pallets for dumping/restoring state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert types here; need to remain compatible with existing custom RPC
| // Max payload size was picked specifically to be large enough to accommodate | ||
| // a CIDv1 using SHA2-256, but too small to accommodate CIDv1 w/SHA2-512. | ||
| // This is purely so that we can test the error condition. Real world configuration | ||
| // should have this set large enough to accommodate the largest possible CID. | ||
| // Take care when adding new tests for on-chain (not IPFS) messages that the payload | ||
| // is not too big. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was an erroneous assumption. Since the configured value would never be this small on mainnet or testnet, it's better to allow a CIDv1/SHA2-512 by size, and have it fail for CID validation instead of for size reasons.
| /// Helper function to generate message payloads | ||
| /// | ||
| /// # Arguments | ||
| /// * `num_items` - Number of message to include in payload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seemed overly complicated--a payload is just an array of bytes; why all the JSON stuff?
pallets/messages/src/lib.rs
Outdated
| let cid = Cid::read_bytes(&cid_b[..]).map_err(|_| Error::<T>::InvalidCid)?; | ||
| ensure!([SHA2_256, BLAKE3].contains(&cid.hash().code()), Error::<T>::InvalidCid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When cleaning up the mocks for the tests, noticed that it was attempting to test failure to accept CIDv1 (SHA2-512) by setting the max payload length artificially small.
The more correct approach (and what was missing from the code, eclipsed by that artificial test scenario) is to reject any CIDs with an unsupported hash. DSNP supports ONLY SHA2-256 and Blake3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is cool, I should do that in Provider Context as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if you like, would you like to make similar changes for cid validation for msa pallet, since this is an enhancement nevertheless, might refactor some?
|
|
||
| /// Storage for messages | ||
| #[pallet::storage] | ||
| pub(super) type MessagesV2<T: Config> = StorageNMap< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Might have to retain (if downstream consumers are tied to this state)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would not be ideal; the number of keys is very large.
To my knowledge, this is only used in:
- Gateway (content-watcher)
- Possibly the Freesky content viewer tool that @wesbiggs built
I will file issues in both of these repos.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is not really net-new; I just haven't merged from main in a while. I've added some additional consts, and moved the CID validation function and tests from the messages pallet.
8a4ff8c
into
feat/schemas-permissions-development
Goal
The goal of this PR is to migrate the
messagespallet to addschema_idto message storage, and useintent_idas the index instead.Existing index values do not need to be changed, as the
schemaspallet migration ensures that the chain is seeded with Intent IDs that match existing Schema IDsRelated to #2581
Checklist